Fix DDL failure for hasMany collections of enums - #16052
Conversation
HibernateBasicProperty.getTable() always resolved to the owning entity's table instead of the collection's join table, so the enum element column for a hasMany-of-enum property was bound to the wrong table: it appeared (wrongly) on the owner and was missing from the join table's own CREATE TABLE, breaking schema generation. Separately, HibernateToManyProperty.joinTableColumName() derived the column name from the enum's fully-qualified class name instead of its simple name. Also introduces HibernateBasicEnumProperty so hasMany-of-enum elements are recognized as HibernateEnumProperty like their singular counterparts, collapsing EnumTypeBinder down to one bindEnumType() entry point and letting each HibernateEnumProperty implementation supply its own table, column name, nullability, and enum-storage configuration instead of the binder branching on property shape. Fixes #16051 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes Hibernate schema-generation (DDL) failures for hasMany collections of enums in grails-data-hibernate7 by ensuring enum collection elements bind their column to the join table (not the owning entity table) and by stabilizing the derived element column name (simple enum name vs fully-qualified name). It also refactors enum binding so both singular-enum properties and enum-collection elements flow through a single EnumTypeBinder.bindEnumType(...) path, with each HibernateEnumProperty implementation providing its enum/table/column/nullability details.
Changes:
- Fix basic collection element table resolution so enum element columns are added to the collection join table rather than the owning entity table.
- Correct join-table element column naming for enum collections to use the enum simple name instead of the fully-qualified name.
- Consolidate enum binding into a single binder entry point and add/adjust tests to cover the DDL and binding behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateBasicProperty.java | Overrides getTable() to use the collection join table when binding basic collection elements. |
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateBasicEnumProperty.java | New enum-collection-element property type implementing HibernateEnumProperty for correct enum binding metadata. |
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateEnumProperty.java | Expands the marker interface to supply enum type, column name, and nullability for a unified binder path. |
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateToManyProperty.java | Fixes enum join-table element column naming to use the enum simple name. |
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateMappingFactory.groovy | Creates HibernateBasicEnumProperty for enum basic collections so they participate in enum binding correctly. |
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/GrailsPropertyBinder.java | Prevents enum-collection properties from bypassing collection binding (ensures join table creation still happens). |
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/EnumTypeBinder.java | Collapses enum binding to bindEnumType(HibernateEnumProperty, path) and delegates configuration to the property + GrailsEnumType. |
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/util/GrailsEnumType.java | Centralizes enum BasicValue configuration (STRING/ORDINAL/IDENTITY) behind a configure(...) method. |
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/BasicCollectionElementBinder.java | Routes enum collection elements through the unified enum binder API. |
| grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/EnumHasManyDdlSpec.groovy | New regression test reproducing #16051: join table contains the enum element column; owner table does not; save/reload works. |
| grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/EnumTypeBinderSpec.groovy | Updates enum binder tests for the unified bindEnumType(...) API and enum-collection property shape. |
| grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/BasicCollectionElementBinderSpec.groovy | Updates collection element binder tests to expect delegation via bindEnumType(...). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16052 +/- ##
==================================================
+ Coverage 51.4879% 52.3221% +0.8342%
- Complexity 17761 18534 +773
==================================================
Files 2039 2039
Lines 95537 97498 +1961
Branches 16571 17138 +567
==================================================
+ Hits 49190 51013 +1823
+ Misses 39042 38998 -44
- Partials 7305 7487 +182 🚀 New features to boost your workflow:
|
jdaugherty
left a comment
There was a problem hiding this comment.
I had AI review this and think it's comments are relevant. Take a look and then I'll take another pass.
…ascade rules PropertyBinder excluded any HibernateEnumProperty from cascade computation via !(instanceof HibernateEnumProperty) - a check that was always vacuous before this PR (no enum type was ever also an Association) until HibernateBasicEnumProperty became both, silently dropping cascade="all" for hasMany-of-enum collections. Delete the now-armed, always-was-pointless clause; instanceof Association<?> alone was always the correct and sufficient guard. While fixing it, move the implied-cascade computation for every to-many shape (Basic, Map-typed, EmbeddedCollection, OneToMany, ManyToMany) onto HibernateToManyProperty itself, which already self-classifies via isBasic()/isManyToMany()/isOneToMany(). This replaces CascadeBehaviorFetcher's external instanceof dispatch across 5 concrete types with the property answering for itself, leaving CascadeBehaviorFetcher only the to-one/embedded/hasOne shapes it still owns. Behavior-preserving: the full CascadeBehaviorFetcherSpec table passes unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Replace the loose it.contains('answer') check with an exact column
set: the old FQN-based naming bug also satisfied contains('answer'),
so the assertion would have gone green against the original bug.
- Fix a resource leak: the PreparedStatement/ResultSet were never
closed, and the ResultSet was drained after doReturningWork
returned. Extract a shared columnNamesFor() helper that does the
whole read inside the callback with try-with-resources.
- Add coverage for the branches this PR actually rewrote: enumType:
'ordinal' storage, and an explicit joinTable column: name.
- Pin the (intentional) nullability change: a hasMany-of-enum element
column stays nullable even when nullable: false is declared,
matching the non-enum sibling collection path.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
!(currentGrailsProp instanceof HibernateToManyProperty) reads as "enums that are not to-many" when the real intent is "not the basic-collection variant", and silently depends on HibernateBasicProperty implementing HibernateToManyCollectionProperty - not obvious at the call site, and the same shape of coupling that caused the cascade regression fixed earlier. Add HibernateEnumProperty.isCollectionElement() (default false, overridden true on HibernateBasicEnumProperty) so the binder asks the property directly, matching the pattern the rest of this PR already follows for column naming and nullability. Also fixes HibernateEnumProperty's stale class Javadoc: it described itself as a marker interface (it now carries default methods) whose Java type is an enum (false for the hasMany-of-enum case). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GrailsPropertyBinder.bindProperty's instanceof HibernateCustomProperty branch had no test that actually reached it: "Test bind custom property type" uses a plain String field with an explicit type: mapping, which GORM classifies as HibernateSimpleProperty (Custom vs. Simple is decided by whether the property's Java type has a registered CustomTypeMarshaller, not by the type: DSL keyword) and which is intercepted earlier by isUserButNotCollectionType() regardless. Tightened that test's assertions to say what it actually verifies, and added a test that constructs a genuine HibernateCustomProperty the way HibernateMappingFactory#createCustom does (no type: mapping), proving it reaches its own branch and not isUserButNotCollectionType() or HibernateSimpleProperty. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@borinquenkid have you finished working through the feedback on this PR? Can I take another look yet? |
@jdaugherty yes |
jdaugherty
left a comment
There was a problem hiding this comment.
Approving — the fix is correct and now properly guarded. Reverting joinTableColumName to getName() fails three EnumHasManyDdlSpec features, so the regression is genuinely pinned. The round-one items all landed and I've resolved those threads: EnumHasManyDdlSpec asserts exact column names and reads inside doReturningWork with try-with-resources, the joinTable column: and nullable: false cases are there, the GrailsPropertyBinder guard moved onto the interface as isCollectionElement(), and the HibernateEnumProperty javadoc is accurate. The extra HibernateCustomProperty coverage in GrailsPropertyBinderSpec is a good addition too.
Everything below is non-blocking follow-up — none of it is a correctness problem and none of it should hold up the fix. Findings are verified by mutation: I broke the production code and checked whether the suite noticed, rather than inferring from reading.
Three tests that don't assert what they're named for
EnumHasManyDdlSpec, the ordinal feature. SubstitutingenumType: 'string'for'ordinal'leaves it green. Column names are identical under both styles, and the round trip reads through the mapping it wrote.HibernateMappingFactorySpec. Deleting theHibernateBasicEnumPropertybranch outright leaves it 29/29 green — all threecreateBasicCollectionfeatures assert the supertype.HibernateToManyPropertySpec(carried over from round one, still unchanged — I've left that thread open). ItsjoinTableColumNamefeature asserts!= nulland passes against the bug it is named for.
None of these are uncovered behaviour — the suite catches all three regressions elsewhere, which is why they stayed green. They're tests that wouldn't fail if the thing they're named for broke, and each is a small fix. Happy for them to land as a follow-up rather than here.
One design question
HibernateBasicProperty.getTable() is overridden on the shared base class, but only the enum path reads it, and it changes what TableForManyCalculator.getJoinTableSchema() sees. That still yields the right schema, but only because CollectionType.create() seeds collectionTable to the owner's table before getJoinTableSchema() runs. Details inline — worth deciding deliberately rather than inheriting, but not a blocker.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (1)
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/EnumTypeBinder.java:99
- EnumTypeBinder.bindEnumType() only applies ColumnConfig from pc.getColumns(), which is correct for scalar enum properties but skips the joinTable column config for hasMany-of-enum collection elements (where the relevant config is PropertyConfig.joinTable.column). This drops index/DDL settings that BasicCollectionElementBinder previously applied via getColumnConfigOptional(). Consider selecting the ColumnConfig based on property.isCollectionElement() and (for collections) reading it from HibernateToManyProperty.getColumnConfigOptional().
if (!pc.getColumns().isEmpty()) {
ColumnConfig columnConfig = pc.getColumns().get(0);
indexBinder.bindIndex(columnName, column, columnConfig, t);
columnConfigToColumnBinder.bindColumnConfigToColumn(column, columnConfig, pc);
}
…specs - Move the getTable() override from HibernateBasicProperty to HibernateBasicEnumProperty: only EnumTypeBinder binds through getTable(), so scalar basic collections no longer inherit the redirect they never needed. - Stop TableForManyCalculator.getJoinTableSchema() reading property.getTable(), which for a basic collection depends on collection-binding order; ask the owning entity's persistent class directly instead. - Restore the table-per-hierarchy "forced to nullable" debug message lost in the EnumTypeBinder refactor. - Harden specs flagged as passing against the bug: pin the derived enum join column name in HibernateToManyPropertySpec, assert HibernateBasicEnumProperty (and its negative) in HibernateMappingFactorySpec, assert raw stored ordinal vs name in EnumHasManyDdlSpec and split its mixed features, cover the hasMany-of-enum fall-through in GrailsPropertyBinderSpec, pin both arms of the getTable ternary in HibernateBasicPropertySpec, and cover the owner-schema fallback in TableForManyCalculatorSpec. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
✅ All tests passed ✅🏷️ Commit: a598d28 Learn more about TestLens at testlens.app. |
|
@jdaugherty nudge |
|
Given how long this has been out there and the critical nature of it, I'm going to merge it. |
Summary
HibernateBasicProperty.getTable()always resolved to the owning entity's table instead of the collection's join table, so ahasMany-of-enum property's element column was bound to the wrong table: it appeared (wrongly) on the owner and was missing from the join table's ownCREATE TABLE, breaking schema generation.HibernateToManyProperty.joinTableColumName()derived the enum element column name from the enum's fully-qualified class name instead of its simple name.HibernateBasicEnumPropertysohasMany-of-enum elements are recognized asHibernateEnumPropertylike their singular counterparts, collapsingEnumTypeBinderdown to a singlebindEnumType()entry point and letting eachHibernateEnumPropertyimplementation supply its own table, column name, nullability, and enum-storage configuration instead of the binder branching on property shape.Fixes #16051
Test plan
EnumHasManyDdlSpecreproducing the issue: join table gets its element column, owner table gets no spurious column, and ahasMany-of-enum can be saved and reloaded end-to-end.EnumTypeBinderSpecandBasicCollectionElementBinderSpecfor the collapsedEnumTypeBinderAPI.:grails-data-hibernate7-core:testsuite green (3009 tests, 0 failures).🤖 Generated with Claude Code